unistd: adding linux(glibc)/freebsd close_range. #2525
Conversation
|
I restarted the Linux/glibc CI as the error is not related to the PR |
|
FreeBSD issue will be fixed once this PR is merged and released. |
Allows to close a range of file descriptors, can set close-on-exec on these and/or unsharing (as having its own file descriptors table after fork for the calling process).
devnexen
force-pushed
the
close_range
branch
from
4c1f57d
to
e67aeaa
Compare
There was a problem hiding this comment.
Hi, I have left some comments.
About this interface, there was an attempt to add it to Nix, #2153, based on the discussion there, this function should be marked unsafe given that it is easy to incur a double close. And, I think it should take raw file descriptors, a common use case is to close all the file descriptors: close_range(0, u32::MAX, 0), using I/O-safe types makes the interface hard to use in this case. Your thoughts on this?
src/unistd.rs
Outdated
| #[cfg(all(target_os = "linux", target_env = "gnu"))] | ||
| /// Unshare the file descriptors range before closing them | ||
| CLOSE_RANGE_UNSHARE; | ||
| /// Set the close-on-exec flag on the file descriptors range |
There was a problem hiding this comment.
Let's be explicit:
| /// Set the close-on-exec flag on the file descriptors range | |
| /// Set the close-on-exec flag on the file descriptors range instead of closing them. |
src/unistd.rs
Outdated
| #[cfg_attr(docsrs, doc(cfg(feature = "fs")))] | ||
| pub struct CloseRangeFlags : c_uint { | ||
| #[cfg(all(target_os = "linux", target_env = "gnu"))] | ||
| /// Unshare the file descriptors range before closing them |
There was a problem hiding this comment.
Considering that the doc in the man page is kinda misleading, let's fix it on our side:
| /// Unshare the file descriptors range before closing them | |
| /// Unshare the file descriptor table, then close the file descriptors specified in the range. |
src/unistd.rs
Outdated
| libc_bitflags! { | ||
| /// Options for close_range() | ||
| #[cfg_attr(docsrs, doc(cfg(feature = "fs")))] | ||
| pub struct CloseRangeFlags : c_uint { |
There was a problem hiding this comment.
This is interesting, libc defines these 2 constants as unsigned integers. 🤔
src/unistd.rs
Outdated
|
|
||
| cfg_if! { | ||
| if #[cfg(all(target_os = "linux", target_env = "gnu"))] { | ||
| libc::syscall(libc::SYS_close_range, fdbegin.as_fd().as_raw_fd() as u32, fdlast.as_fd().as_raw_fd() as u32, flags.bits() as i32) |
There was a problem hiding this comment.
glibc added the wrapper in 2.34, and the libc crate has it exposed, any reason why we are using a raw syscall here?
There was a problem hiding this comment.
Originally I call the wrapper but I think it is due to the CI when there is old glibc
There was a problem hiding this comment.
Makes sense, some CIs are still using Ubuntu 20.04 due to this issue, which has glibc 2.31 😮💨, perhaps we can try bumping them to see if we are lucky now
There was a problem hiding this comment.
we can do that if we do not need to bother supporting older releases out in the field. I may come back to it later.
There was a problem hiding this comment.
Yeah, if we use glibc wrapper, then using this interface would require glibc 2.34 or above. We are still not lucky regarding that test: #2533, perhaps we should disable the test under QEMU then 😪
src/unistd.rs
Outdated
| all(target_os = "linux", target_env = "gnu"), | ||
| target_os = "freebsd" | ||
| ))] | ||
| pub fn close_range<F: std::os::fd::AsFd>(fdbegin: F, fdlast: F, flags: CloseRangeFlags) -> Result<Option<c_int>> { |
There was a problem hiding this comment.
I am curious why it returns a Result<Option<c_int>>, and the reason behind the code related to Errno. From the man page, it returns -1 on error, and 0 on success, looks like it is just a normal syscall👀
There was a problem hiding this comment.
I agree with @SteveLauC on all points. Additionally, the "fix tests" commit appears to be unrelated to this PR. If it is indeed unrelated, then please open a separate PR for it, and use a more descriptive commit message.
test/test_unistd.rs
Outdated
| tempfile3.write_all(CONTENTS).unwrap(); | ||
| tempfile2.write_all(CONTENTS).unwrap(); | ||
| tempfile1.write_all(CONTENTS).unwrap(); | ||
| let areclosed = close_range(tempfile1, tempfile3, CloseRangeFlags::CLOSE_RANGE_CLOEXEC); |
There was a problem hiding this comment.
In a multithreaded program, which this is, there is no guarantee that tempfile3's file descriptor will be higher than tempfile1's.
Allows to close a range of file descriptors, can set close-on-exec on these and/or unsharing (as having its own file descriptors table
after fork for the calling process).
What does this PR do
Checklist:
CONTRIBUTING.md